Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make this package implementation-agnostic #573

Merged
merged 6 commits into from
Aug 1, 2018

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented May 25, 2018

Rather than always loading Node Sass, this now requires users to pass in
either Dart Sass or Node Sass as an option to the loader.

Closes #435

@jsf-clabot
Copy link

jsf-clabot commented May 25, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 25, 2018

Codecov Report

Merging #573 into master will increase coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #573      +/-   ##
==========================================
+ Coverage   97.54%   97.81%   +0.26%     
==========================================
  Files           6        6              
  Lines         122      137      +15     
==========================================
+ Hits          119      134      +15     
  Misses          3        3
Impacted Files Coverage Δ
lib/loader.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 714f5c6...41b0079. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for PR! Good work!

CHANGELOG.md Outdated
}]
}
};
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working docs, but please remove this, we can do this before release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

README.md Outdated
{
loader: "sass-loader", // compiles Sass to CSS
// require("node-sass") instead to use Node Sass
options: {sass: require("sass")}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be option. sass-loader should resolve node-sass, then try to resolve dart-sass, if not found nothing - throw error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? Not allowing users to explicitly control this will mean that if they (or one of their dependencies if they're using flat dependencies) requires Node Sass for some unrelated reason, they're forced to use Node Sass. This may even result in them unexpectedly switching to Node Sass down the line.

It seems like several users would prefer making this configuration explicit.

Copy link
Member

@alexander-akait alexander-akait May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 Hm, yes you are right it can lead to problems, let's rename sass to implementation and load node-sass by default. It is allow to upgrade loader without break build and avoid unnecessary major.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. It's worth noting though that the name implementation, in addition to being more of a pain to type, means that users can't just write

const sass = require("sass");

// ...
    options: { sass }
// ...

README.md Outdated
loader: "sass-loader",
options: {
sass: require("sass"),
fiber: Fiber
Copy link
Member

@alexander-akait alexander-akait May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to internal behavior (no new options), please provide benchmark for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by "move this to internal behavior". Do you want sass-loader to depend on fibers itself? Be aware that fibers requires native compilation, and may be difficult to install on some systems.

I'm not sure what kind of benchmarks you're looking for, but as the maintainer of Dart Sass, I can assure you that a 2x speed hit is what we measure pretty consistently when using the asynchronous code path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 okey, let's leave this as is

@@ -6,6 +6,7 @@ const path = require("path");
const webpack = require("webpack");
const fs = require("fs");
const merge = require("webpack-merge");
const sass = require("node-sass");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests should be running on node-sass an dart-sass together, we need change test script:

  1. Install node-sass and run tests
  2. Install dart-sass and run tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to do that gracefully, especially if the implementation isn't supplied as an option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 skip tests where dart-sass doesn't have options/features/etc and add comments about this. Why? After release all people starting upgrade and we will get a lot of issues like "It is woks on x.x.x version, but after upgrade to x.x.x it is doesn't works. [A lot of angry words]" (a lot of people don't read changelog and release pages). We should prepare to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but at a higher level I'm not sure how to run these tests with both versions of Sass or how to switch between those versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add additional look here https://github.com/webpack-contrib/sass-loader/blob/master/test/index.test.js#L35 with node-sass and sass values and we need some modify tests to allow using difference implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure I understand. Can you give me an example of what a test you're envisioning would look like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this (pseudo code):

implementations.forEach(implementation => {
  syntaxStyles.forEach(ext => {
    loaderOptions.implementation = implementation
    // tests
  });
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "sass-loader",
"version": "7.0.1",
"version": "8.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

lib/loader.js Outdated
throw new Error("Dart Sass version " + version + " is incompatible with ^1.3.0.");
}
} else if (implementation === "node-sass") {
if (major < 4 || major > 4) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide information why node-sass > 4 is not compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node Sass 5 hasn't been released yet. According to semver, it may introduce arbitrary breaking changes, so it's not safe to eagerly declare compatibility with it.

lib/loader.js Outdated

const implementation = parsed[1];
const version = parsed[2];
const components = /^(\d+)\.(\d+)/.exec(version);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use semver package for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor Author

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem to have made branch coverage on loader.js fall below the threshold, but I'm not sure why since I only changed the conditions without changing any branches.

CHANGELOG.md Outdated
}]
}
};
```
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

README.md Outdated
{
loader: "sass-loader", // compiles Sass to CSS
// require("node-sass") instead to use Node Sass
options: {sass: require("sass")}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? Not allowing users to explicitly control this will mean that if they (or one of their dependencies if they're using flat dependencies) requires Node Sass for some unrelated reason, they're forced to use Node Sass. This may even result in them unexpectedly switching to Node Sass down the line.

It seems like several users would prefer making this configuration explicit.

README.md Outdated
loader: "sass-loader",
options: {
sass: require("sass"),
fiber: Fiber
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by "move this to internal behavior". Do you want sass-loader to depend on fibers itself? Be aware that fibers requires native compilation, and may be difficult to install on some systems.

I'm not sure what kind of benchmarks you're looking for, but as the maintainer of Dart Sass, I can assure you that a 2x speed hit is what we measure pretty consistently when using the asynchronous code path.

lib/loader.js Outdated

const implementation = parsed[1];
const version = parsed[2];
const components = /^(\d+)\.(\d+)/.exec(version);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

lib/loader.js Outdated
throw new Error("Dart Sass version " + version + " is incompatible with ^1.3.0.");
}
} else if (implementation === "node-sass") {
if (major < 4 || major > 4) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node Sass 5 hasn't been released yet. According to semver, it may introduce arbitrary breaking changes, so it's not safe to eagerly declare compatibility with it.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "sass-loader",
"version": "7.0.1",
"version": "8.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -6,6 +6,7 @@ const path = require("path");
const webpack = require("webpack");
const fs = require("fs");
const merge = require("webpack-merge");
const sass = require("node-sass");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to do that gracefully, especially if the implementation isn't supplied as an option.

lib/loader.js Outdated
@@ -62,6 +41,16 @@ function sassLoader(content) {
addNormalizedDependency
));

if (asyncSassJobQueue === null) {
if (!options.sass) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better rename to implementation, sass is very misleading

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And should be node-sass by default (comment above).

lib/loader.js Outdated
* @param {string} info
*/
function validateSassVersion(info) {
const parsed = /^([^\t]+)\t([^\t]+)/.exec(info);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use regex here, please use semver for parsing major and minor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 looks we still use /^([^\t]+)\t([^\t]+)/.exec(info), better use semver.major, semver.minor and semver.patch functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last regex isn't parsing the semver itself, it's parsing the implementation name and version number from the info string. I could also use info.split("\t") if you want to avoid any regexes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 maybe we can do this without regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

README.md Outdated
{
loader: "sass-loader", // compiles Sass to CSS
// require("node-sass") instead to use Node Sass
options: {implementation: require("sass")}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's left default example as is and improve section about dart-sass
Like this:

<h2 align="center">Examples</h2>
### `node-sass` default
// Description and example
### `dart-sass`
// Description and example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that if you want, but from the perspective of the Sass organization, I'd really like to present Dart Sass and Node Sass as equal options. Making Node Sass the default for backwards-compatibility makes sense, but in terms of end-user messaging I think it makes more sense to frame it as something each user should explicitly choose.

Also, from the perspective of end users, I think most people will find a lot of value in starting with Dart Sass because it's easy to install and moving to LibSass if and when they need a performance boost. Presenting the default documented workflow as LibSass first means you'll have some percentage of users whose first interaction with sass-loader is installation woes caused by native compilation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 please do this, we switch on dart sass in next major release. We should try dart sass in wild before release major.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@alexander-akait
Copy link
Member

alexander-akait commented May 29, 2018

Good work! Thanks!

@@ -6,6 +6,7 @@ const path = require("path");
const webpack = require("webpack");
const fs = require("fs");
const merge = require("webpack-merge");
const sass = require("node-sass");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this (pseudo code):

implementations.forEach(implementation => {
  syntaxStyles.forEach(ext => {
    loaderOptions.implementation = implementation
    // tests
  });
})

@@ -6,6 +6,7 @@ const path = require("path");
const webpack = require("webpack");
const fs = require("fs");
const merge = require("webpack-merge");
const sass = require("node-sass");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have the coverage failing by a small margin again, and I'm not sure what to do about that.

README.md Outdated
{
loader: "sass-loader", // compiles Sass to CSS
// require("node-sass") instead to use Node Sass
options: {implementation: require("sass")}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

lib/loader.js Outdated
* @param {string} info
*/
function validateSassVersion(info) {
const parsed = /^([^\t]+)\t([^\t]+)/.exec(info);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -6,6 +6,7 @@ const path = require("path");
const webpack = require("webpack");
const fs = require("fs");
const merge = require("webpack-merge");
const sass = require("node-sass");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@alexander-akait
Copy link
Member

@nex3 don't look on coverage, also please rebase 👍

nex3 added 2 commits June 4, 2018 11:45
Rather than always loading Node Sass, this now requires users to pass
in either Dart Sass or Node Sass as an option to the loader.

Closes webpack-contrib#435
Importing stylesheets without extensions is non-standard behavior
that's not supported in Dart Sass (see sass/libsass#2672).
@nex3
Copy link
Contributor Author

nex3 commented Jun 4, 2018

Done!

@xzyfer xzyfer mentioned this pull request Jun 8, 2018
@nex3
Copy link
Contributor Author

nex3 commented Jun 16, 2018

@evilebottnawi Is there anything left to be done here?

@alexander-akait
Copy link
Member

@nex3 sorry for delay, review tomorrow, a lot of work, thanks for work!

@nex3
Copy link
Contributor Author

nex3 commented Jun 28, 2018

Friendly ping!

// 2. It does not matter whether the CSS-file exists or not, the file is just linked
@import ./does/not/exist.css
// 3. When the .css extension is missing, the file is included just like scss
@import ~css/some-css-module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why some tests will be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS imports are not currently supported by Dart Sass. CSS imports with the .css suffix are also slated for removal from LibSass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 can we left css tests only for node-sass, we remove this after upgrade node-sass to 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how, since all these test files are shared between the two implementations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 need investigate, will be great if you do this, otherwise i can try this too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely don't know how to do it without a huge refactor of how tests are run.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evilebottnawi - do you have any ETA when you might be able to look at this? :) Or, is it possible to move forward without this, as the features being tested are soon to deprecate anyway?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TroyAlford need @jhnns review

@alexander-akait
Copy link
Member

BTW Great work! Thanks!

README.md Outdated
@@ -24,11 +24,17 @@ Looking for the webpack 1 loader? Check out the [archive/webpack-1 branch](https
<h2 align="center">Install</h2>

```bash
npm install sass-loader node-sass webpack --save-dev
npm install sass-loader sass webpack --save-dev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there is dart sass installation if node-sass is the default implementation?

"sass-loader" // compiles Sass to CSS, using Node Sass by default later in an example in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed this. Changed back to node-sass.

Copy link
Member

@jhnns jhnns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, the PR looks pretty good overall 🎉 👍

Besides my comment to fiber and the threadpool, I have another question: Why did you rename test/node_modules/module to test/node_modules/module.scss? The missing file extension was on purpose.

Anyway, I'm pretty confident that we can merge this soon. Thanks again for your hard work 👍

callbacks. To avoid this overhead, you can use the
[`fibers`](https://www.npmjs.com/package/fibers) package to call asynchronous
importers from the synchronous code path. To enable this, pass the `Fiber` class
to the `fiber` option:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since webpack uses the asynchronous compilation, can we automate that process? So that we just require("fiber") and apply the option automatically from normalizeOptions? I guess that every dart-sass user would want to have that speedup. If require("fiber") fails, we could instruct the user to install fiber and point a link to the dart-sass README.

In this README, I'd just mention something like "be sure to have fibers installed in your project, see dart-sass README" (with link).

What do you think?

We'd need to add fibers as dev dependency then.

Copy link

@just-boris just-boris Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fiber is a native package, so including this by default would defeat the value of this change - not having C++ code in dependencies, make SASS as easy to use as Less or Stylus.

The caveats of native packages was one of the reasons to not include SASS by default into create-react-app: facebook/create-react-app#78 (comment)

The current documentation seems fine. When users would ask "compilation of my SASS is too slow, how can I speed this up?", they will be able to find an answer. But making fibers strong recommendation or even mandatory would sound like "You should buy Ferrari when you simply need a car".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @just-boris; there's a lot of value in providing an easy pure-JS set-up experience, and then letting performance-conscious users focus on that when they need it.

Something to consider, probably after this PR lands, would be adding an option to run Dart Sass in synchronous mode, which is efficient without the use of fibers (at the cost of not supporting async importers or custom functions).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fiber is a native package, so including this by default would defeat the value of this change - not having C++ code in dependencies, make SASS as easy to use as Less or Stylus.

Good catch 👍 I totally agree.

Something to consider, probably after this PR lands, would be adding an option to run Dart Sass in synchronous mode, which is efficient without the use of fibers (at the cost of not supporting async importers or custom functions).

Yes, I'm thinking about moving away from async importers. This would be a good fit.

lib/loader.js Outdated
const implementation = options.implementation || require("node-sass");

validateSassVersion(implementation.info);
asyncSassJobQueue = async.queue(implementation.render, threadPoolSize - 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The async job queue mitigates a problem of node-sass/libsass where the threadpool gets exhausted (see sass/node-sass#857 (comment)). Do you know if dart-sass suffers from the same problem? (I don't think so, unless you ask for a thread from libuv like node-sass does)

If not, we could get rid of the queue for dart-sass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe Dart Sass suffers from that problem, but I wanted to keep the code path as similar between the two implementations as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, but I suspect the queue to be one of the performance bottlenecks as discussed in #296. However, that's not a blocker for this PR.

lib/loader.js Outdated
throw new Error("Unknown implementation \"" + implementation + "\".");
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

});
describe("prepending data", () => {
it("should extend the data-option if present", () => execTest("prepending-data", {
data: "$prepended-data: hotpink" + (ext == "sass" ? "\n" : ";")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch 👍

// @see https://github.com/webpack-contrib/sass-loader/issues/368#issuecomment-278330164
{ loader: pathToSassLoader, options: {} }
]
}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for deduping that

describe("source maps", () => {
function buildWithSourceMaps() {
return new Promise((resolve, reject) => {
webpack({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't you re-use runWebpack here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses different values for module.rules, which webpack-merge can't merge in (it would just try to add rules to the list rather than replacing it entirely).

sourceMap.should.have.property("sourceRoot", fakeCwd);
// This number needs to be updated if imports.scss or any dependency of that changes.
// Node Sass includes a duplicate entry, Dart Sass does not.
sourceMap.sources.should.have.length(implementation == nodeSass ? 12 : 11);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of the duplicated dependency. Makes this test easier. Which dependency is duplicated?

} else {
err.message.should.match(/Expected "{"./);
err.message.should.match(/\(line 3, column 1\)/);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the long run it would probably be better to compare these errors with errors that have been snapshotted during createSpec, but that's ok for now I guess.

}, {
implementation: merge(nodeSass, { info: "strange-sass\t1.0.0" })
}, (err) => {
err.message.should.match(/Unknown implementation "strange-sass"\./);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests here 👍

Copy link
Contributor Author

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you rename test/node_modules/module to test/node_modules/module.scss? The missing file extension was on purpose.

That's another situation where you were testing import behavior that's being deprecated in LibSass and isn't supported at all in Dart Sass. See sass/libsass#2672.

callbacks. To avoid this overhead, you can use the
[`fibers`](https://www.npmjs.com/package/fibers) package to call asynchronous
importers from the synchronous code path. To enable this, pass the `Fiber` class
to the `fiber` option:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @just-boris; there's a lot of value in providing an easy pure-JS set-up experience, and then letting performance-conscious users focus on that when they need it.

Something to consider, probably after this PR lands, would be adding an option to run Dart Sass in synchronous mode, which is efficient without the use of fibers (at the cost of not supporting async importers or custom functions).

lib/loader.js Outdated
const implementation = options.implementation || require("node-sass");

validateSassVersion(implementation.info);
asyncSassJobQueue = async.queue(implementation.render, threadPoolSize - 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe Dart Sass suffers from that problem, but I wanted to keep the code path as similar between the two implementations as possible.

describe("source maps", () => {
function buildWithSourceMaps() {
return new Promise((resolve, reject) => {
webpack({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses different values for module.rules, which webpack-merge can't merge in (it would just try to add rules to the list rather than replacing it entirely).

@TroyAlford
Copy link

Just a gentle nudge: what's next, in terms of getting this PR merged and released?

@alexander-akait
Copy link
Member

/cc @nex3 @jhnns

@TroyAlford
Copy link

Really not wanting to be a pest, but there are some downstream additional changes that this gates. :) Any chance of getting some further review / a potential merge soon?

@co3k
Copy link

co3k commented Aug 1, 2018

node-sass depends on node-gyp, but vulnerability handling by node-gyp team is very slow so we really need to have options to use alternative sass implementations. Please consider to merge this pull request.

jhnns added a commit that referenced this pull request Aug 1, 2018
In preparation for #573
Imports without file extensions are deprecated in LibSass and
even not supported in DartSass.
jhnns added a commit that referenced this pull request Aug 1, 2018
In preparation for #573
Imports without file extensions are deprecated in LibSass and
even not supported in DartSass.
@jhnns jhnns merged commit bed9fb5 into webpack-contrib:master Aug 1, 2018
@jhnns
Copy link
Member

jhnns commented Aug 1, 2018

I've re-added the CSS tests for the node-sass implementation. I've also removed the queue for DartSass.

@jhnns
Copy link
Member

jhnns commented Aug 1, 2018

Shipped with v7.1.0 🚀
Thanks for your great work @nex3 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants